Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RSDK-9864] Remove gostream middle-man dependency from webcam #4604

Merged
merged 75 commits into from
Feb 3, 2025

Conversation

hexbabe
Copy link
Member

@hexbabe hexbabe commented Dec 5, 2024

RSDK-9864

Remove gostream middle-man dependency from webcam

Tested webcam on Mac and RPI4. Live and GetImage views on app. Unplug + replug on RPI4 (Logitech C90 Pro). Driver selection on app doesn't work anymore (as expected)

hexbabe and others added 30 commits October 24, 2024 14:59
…here we convert to go image; Change default mimetypes for classifier
…ng default mimetypes for vision since we are failing unit tests with 'do not know how to encode' errors
}
}

for _, d := range needToClose {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just close the driver after getting Properties in the main loop?

Does not seem like that big of a deal if we need to open everything then close out afterwards as is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reasonable suggestion... but I'm a bit hesitant touching this code since it's stuff copy and pasted from gostream. Feels a bit out of scope and could cause confusion since there's no git history of it being here. WDYT @randhid

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Sean Yu, we can address more code refactoring/optimizations for webcam when we have time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

cam, err := tryWebcamOpen(ctx, conf, path, false, constraints, logger)
if err != nil {
return nil, "", errors.Wrap(err, "cannot open webcam")
if conf.Width != 0 && conf.Height != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be checking conf width and height here? Could the conf specify 0s but the driver that is matched has a valid size?

Copy link
Member Author

@hexbabe hexbabe Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're so right let me fix it to pull a frame regardless of config

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 30, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 31, 2025
@randhid
Copy link
Member

randhid commented Jan 31, 2025

Note: we should add back the known cameras <> camera intrinsics/distortion map at some point, to make it easy for whoever is using those without configuring instrinsics explicitly in their config.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 31, 2025
@hexbabe
Copy link
Member Author

hexbabe commented Jan 31, 2025

Note: we should add back the known cameras <> camera intrinsics/distortion map at some point, to make it easy for whoever is using those without configuring instrinsics explicitly in their config.

did we have this at some point?

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 31, 2025
@randhid
Copy link
Member

randhid commented Jan 31, 2025

Note: we should add back the known cameras <> camera intrinsics/distortion map at some point, to make it easy for whoever is using those without configuring instrinsics explicitly in their config.

did we have this at some point?

Yes, and in your pr we still have the embedded data.json and the intirnsics logger, but I wonder if we've lost the glue that initialize a camera without explicit intrinsics and extrinsics in the config even if a user doesn't include them. Anyway something for another day.

@hexbabe
Copy link
Member Author

hexbabe commented Jan 31, 2025

but I wonder if we've lost the glue that initialize a camera without explicit intrinsics and extrinsics in the config even if a user doesn't include them

as in: try to use these jsons in the case no intrinsics are specified

@randhid
Copy link
Member

randhid commented Jan 31, 2025

but I wonder if we've lost the glue that initialize a camera without explicit intrinsics and extrinsics in the config even if a user doesn't include them

as in: try to use these jsons in the case no intrinsics are specified

Yep

@randhid
Copy link
Member

randhid commented Feb 3, 2025

You can merge this in with my webcam removals. I'll close that pr #4720

@hexbabe
Copy link
Member Author

hexbabe commented Feb 3, 2025

You can merge this in with my webcam removals. I'll close that pr #4720

OK so merge this when discovery card is live?

@randhid
Copy link
Member

randhid commented Feb 3, 2025

You can merge this in with my webcam removals. I'll close that pr #4720

OK so merge this when discovery card is live?

It is already - I am more interested in the map of intrinsics for known cameras being preserved. Is there a way to do so in this PR, as there is no real rush to merge this yet?

@randhid
Copy link
Member

randhid commented Feb 3, 2025

You can merge this in with my webcam removals. I'll close that pr #4720

OK so merge this when discovery card is live?

It is already - I am more interested in the map of intrinsics for known cameras being preserved. Is there a way to do so in this PR, as there is no real rush to merge this yet?

I forgot I said for another day - let's merge this in and make a follow-up ticket.

@hexbabe hexbabe merged commit 827a907 into viamrobotics:main Feb 3, 2025
16 checks passed
@hexbabe hexbabe deleted the RSDK-9204-experimental branch February 3, 2025 16:07
@hexbabe
Copy link
Member Author

hexbabe commented Feb 3, 2025

You can merge this in with my webcam removals. I'll close that pr #4720

OK so merge this when discovery card is live?

It is already - I am more interested in the map of intrinsics for known cameras being preserved. Is there a way to do so in this PR, as there is no real rush to merge this yet?

I forgot I said for another day - let's merge this in and make a follow-up ticket.

https://viam.atlassian.net/browse/RSDK-9894

vijayvuyyuru pushed a commit to vijayvuyyuru/rdk that referenced this pull request Feb 11, 2025
…botics#4604)

Co-authored-by: nicksanford <[email protected]>
Co-authored-by: Dan Gottlieb <[email protected]>
Co-authored-by: Ethan Look-Potts <[email protected]>
Co-authored-by: abe-winter <[email protected]>
Co-authored-by: Eliot Horowitz <[email protected]>
Co-authored-by: Naomi Pentrel <[email protected]>
Co-authored-by: Maxim Pertsov <[email protected]>
Co-authored-by: nicksanford <[email protected]>
Co-authored-by: Devin Hilly <[email protected]>
Co-authored-by: martha-johnston <[email protected]>
Co-authored-by: Kurt S <[email protected]>
Co-authored-by: randhid <[email protected]>
Co-authored-by: randhid <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.